Skip to content

Add Suspense and ErrorBoundary to better support suspending from hooks #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

mpeyper
Copy link
Member

@mpeyper mpeyper commented Apr 5, 2019

What:

Added support for hooks that suspend during rendering.

Why:

We broke it when we started catching thrown values with the false assumption that they were always errors.

How:

  1. Added a Suspense and fallback to allow the thrown promise to be handled
  2. Moved the error handling into an ErrorBoundary to make it more "reacty"

Checklist:

  • Documentation updated
  • Tests
  • Typescript definitions updated
  • Ready to be merged
  • Added myself to contributors table

Fixes #27

return null
}

class ErrorBoundary extends React.Component {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wish there was a non-class way to handle error boundaries

@codecov-io
Copy link

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #35   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          28     37    +9     
  Branches        1      3    +2     
=====================================
+ Hits           28     37    +9
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70eae59...f5914f4. Read the comment docs.

expect(result.current).toBe('Bob')
})

test('should set error if suspense promise rejects', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this test actually adds much value. in #27 I discovered that a rejected Promise does not trigger the error boundary, and instead it's up to the suspending code to throw the error on the subsequent render (see line 24 for example), which means this is actually no different than if the hook just threw an error for some other reason (like we already do in the errorHook test suite.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone who implements a Suspense library will be doing such so I think this provides value

Copy link
Member Author

@mpeyper mpeyper Apr 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my thought too and why I kept it when cleaning up the commits.

@mpeyper
Copy link
Member Author

mpeyper commented Apr 5, 2019

@ntucker Please submit a PR adding yourself as a contributor. You definitely get the bug contribution for #27, but I feel like you also need something for the help you gave me in getting my test cases set up properly (test or examples?). If you want to review this PR you can have a review contribution too 😉.

import { render, cleanup, act } from 'react-testing-library'

function TestHook({ callback, hookProps, children }) {
try {
Copy link
Contributor

@ntucker ntucker Apr 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you kept this block an just re-threw you could also grab any promises to store in a third result value of 'promise' or the like. While waitForNextUpdate() will work in most cases, inspecting the promise thrown would be useful in test cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If you rethrow the exact same error it will preserve the original stack trace.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention of removing this and using a Suspense and ErrorBoundary wrapper was to protect this library from having to deal with the internals of how error handling and suspended rendering actually works. The fact that a thrown promise is what triggers react to suspend is an implementation for React, rather than something we should be relying on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're still setting error property though, right? Seems like you are somewhat depending on react in that manner. regardless - this is a react hooks testing library. very specific to how react works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, error is being set, but it's coming from the exposed componentDidCatch API, so we don't care how React gets it to that point, just deal with it when it gets there. When we look at the Suspense equivalent, React does not expose this to us in any way, so if we want to handle it, we essentially need to recreate react's internal logic in order to treat it the same way. If React changes the specifics on how it deals with promises, we would also need to change.

Copy link
Member Author

@mpeyper mpeyper Apr 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if waitForNextUpdate() works

It should. I'd rather fix holes where it doesn't than maintain custom suspense handling.

  • the promise to be caught so it doesn't throw above

This solution would not end up throwing out of the renderHook function, if that's what you mean?

  • a mechanism to see when it is called

Can you unpack this a bit more? Are you concerned about when an update was triggered by something other than the promise resolving, or something else?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One path is that no promise is thrown from the hook - I want to test that it actually threw a promise, and THEN did something else

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm questioning the value of having that much pwoer to inspect the internals of what's happening in your hook (i.e. white box test) vs. just asserting the output from the given inputs (i.e. black box test).

In my experience, white box test are very fragile to changes in the system and need constant updating as the internals shift around (e.g. you might change the condition for when the promise is thrown, so now would need to move things around in your tests to match the change). The black box tests are more likely to continue to work so, long as the contract of the inputs and outputs hasn't changes (otherwise the test must be changed anyway in both testing styles).

In cases where you need more control over when and how the promise thrown, then you may be better off extracting it into some form of factory pattern and mocking that behaviour in the hook test, and testing those variations of the factory seperately to the hook itself.

As an escape hatch, if you really need access to the promise itself, I think you could just catch it in the hool callback itself

let promise
const { result } = renderHook(() => {
  promise = null // assuming you want to reset it between renders
  try {
    useSomething()
  } catch (e) {
    if (e.then) {
      promise = e
    }
  }
})

const promiseValue = await promise

expect(promiseValue).toBe('whatever')

I'm still unsure how that is more useful than

const { result, waitForUpdate } = renderHook(() => useSomething())

await waitForUpdate

expect(result.current).toBe('whatever you expect after the prmise resolved')

As in both cases

  • if the promise resolves, and the expectation is met, the the test passes
  • if the promise resolves, and the expectation is not met, the test fails
    • in the first example all you would know the promise value changed
    • in the second example you would only know something went wrong to affect the output
  • if the promise rejects, the test fails with the thrown error
  • if the promise is not thrown, the test will fail
    • in the first example it would probably be about trying to await null
    • in the second example the test would timeout waiting for an update that never comes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also leaning away from explicitly handling supsense as it would get very difficult to manage if you had a hook throw a promise, and then once resolved throw another promise, etc. Suspense would keep that component suspending untill it eventually rendered something, whereas keeping track of each one and what they were up to could get complicated.

Copy link
Contributor

@ntucker ntucker Apr 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, I would argue that suspense is very much an explicit interface and not an implementation detail - as anyone using the hook that throws promises will need to define suspense boundaries themselves. It's not about how but if.

Yes it could throw many times - that should be up to the test writer to understand exactly what that behavior should be - or if it even cares at all. I agree in many cases you won't care, and you can simply waitForUpdate. But there are certainly cases where you will.

(I responded slowly this time due to company hackathon last week and then I was out traveling over weekend)

@mpeyper
Copy link
Member Author

mpeyper commented Apr 16, 2019

@ntucker I'm going to merge this as-is for now as I'm aware that no suspense compatible hooks can be tested int he current version and what we are discussing would probably be additive to this change anyway. Please feel free to raise a new issue referencing this PR and #27 if you want to discuss it further.

Also, I'm still waiting for that PR to add yourself as a contributor. If you don't want to do it, that's fine, or if you're happy, I can make the addition for you (but you won't get the github cred if I do it).

@mpeyper mpeyper merged commit cfb0345 into master Apr 16, 2019
@ntucker
Copy link
Contributor

ntucker commented Apr 16, 2019

Ya that seems reasonable. This is definitely an improvement that should be released for people to use.

@mpeyper mpeyper deleted the suspense-support branch June 8, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to test hooks that Suspend
3 participants